Add conditional coverage flags for accurate platform coverage #1260

rmartin16 · 2023-05-03T14:40:01Z

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

rmartin16 · 2023-05-05T02:57:23Z

100% coverage on all platforms/versions. Still need to ensure everything is coherent and probably some docs updates.

@freakboy3742, if you have a second, curious if you have thoughts on this failure? maybe just the fact this is a 3.12 alpha?

https://github.com/beeware/briefcase/actions/runs/4889292359/jobs/8727757471

Combined data file .coverage.macOS.3.12-dev.Mac-1683252986482.local.4569.784886
Wrote HTML report to htmlcov/index.html
Name                                        Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------------------------
src/briefcase/platforms/macOS/__init__.py     251      0    107      1  99.7%   572->exit
---------------------------------------------------------------------------------------
TOTAL                                        5497      0   1876      1  99.9%

freakboy3742 · 2023-05-05T05:07:05Z

@freakboy3742, if you have a second, curious if you have thoughts on this failure? maybe just the fact this is a 3.12 alpha?

~~That... makes no sense to me at all. Unless I'm misreading things, the line it's referencing doesn't even seem like a candidate for execution~~ Ignore me - I see now I was looking at the wrong hash.

My guess would be that this is a variation of the nedbat/coveragepy#1572 problem, but I'm not clear why that's a problem here.

freakboy3742 · 2023-05-05T05:18:34Z

.github/workflows/ci.yml

      uses: actions/[email protected]
      with:
        name: coverage-data
        path: ".coverage.*"
        if-no-files-found: ignore

-    - name: Report platform coverage
-      run: tox -e coverage
+  coverage-platform:


If what we're generating is a "platform and python version" specific coverage report, and it will pass with 100% on every platform/python combination... is there any reason to not tag this to the end of the testing pass for a platform and python, rather than doing it as a separate step with it's own matrix fanout?

Fair point. Organic evolution of this PR has definitely influenced this....however, I did want to ensure that incomplete coverage for a single platform/version doesn't abort all of CI. I think the better experience for contributors is the complete picture of coverage when tests are passing.

Thinking about it briefly, failing coverage in the same job as tests will probably require allowing all tests to complete even if tests on one platform fail in order to allow all coverage for all platforms to be available even if only one platform/version fails.

So, some trade-offs involved, I suppose. I will say that checking coverage after the fact definitely increases how long CI runs....(at least until we can prevent having to install all dev dependencies just to run coverage).

In addition to wanting to provide a full analysis of coverage before failing, the only way to assess coverage across Python versions for a particular platform is to coalesce the coverage files at the end.

But this must be done using the OS for that platform. So, we end up with at least the 3 "Platform coverage" jobs alongside "Project coverage" after the tests.

At that point, if those 3 additional coverage jobs are necessary, the benefit of performing specific Python version coverage there becomes more palatable to 1) allow their failure to fail all of CI but not fail testing and 2) provide better consolidation of the coverage analysis in CI.

P.S.

But [platform-specific coverage] must be done using the OS for that platform

This is necessary in order for the new conditional coverage rules to work. They operate on sys.platform to know when to require/ignore certain code blocks. We could consider spoofing this, though, to potentially allow coverage analysis of a platform while not on that platform. I can at least experiment with that.

This turned out to be a fruitful experiment; I was able to incorporate python version-specific and platform-specific coverage reporting in-line with the existing job matrices.

Although, this has two larger status-quo changes:

Unit tests no longer fast fail

This is to prevent coverage reporting from failing a job and thus the whole matrix.

I think ideally developers will have access to the full coverage information.

The Verify App Build tests will not run now until unit tests and python version-specific coverage is passing.

Since failures for unit tests and python-specific coverage are represented by a single flag, either failure prevents app builds.

I haven't found an existing flag to avoid this....of course, we could always create one, I guess.

Conditional Coverage Reporting with Platform Spoofing #1262

rmartin16 · 2023-05-05T14:41:15Z

src/briefcase/platforms/macOS/__init__.py

        finally:
            # Ensure the App also terminates when exiting
-            if app_pid:
+            if app_pid:  # pragma: no-cover-if-is-py310
                with suppress(ProcessLookupError):
                    self.tools.os.kill(app_pid, SIGTERM)


I...don't know what's happening on 3.10 here...

rmartin16 · 2023-05-05T14:42:45Z

tests/commands/dev/test_get_environment.py


 import pytest

 PYTHONPATH = "PYTHONPATH"
 PYTHONMALLOC = "PYTHONMALLOC"


+@pytest.mark.parametrize("platform", ["windows", "darwin", "linux", "wonky"])
+def test_pythonmalloc_only_set_for_windows(


This is to get branch coverage on any individual platform.

rmartin16 · 2023-05-06T01:17:14Z

Closing in favor of #1262

rmartin16 force-pushed the cond-cov branch from fe7581a to 0eeb313 Compare May 3, 2023 14:51

rmartin16 changed the title ~~Add conditional coverage flags for accurate coverage anywhere~~ Add conditional coverage flags for accurate platform coverage May 3, 2023

rmartin16 force-pushed the cond-cov branch 28 times, most recently from fb879f8 to bd2e2d0 Compare May 4, 2023 18:09

rmartin16 force-pushed the cond-cov branch 19 times, most recently from ad60123 to 80a1cda Compare May 5, 2023 02:17

freakboy3742 reviewed May 5, 2023

View reviewed changes

rmartin16 commented May 5, 2023

View reviewed changes

rmartin16 force-pushed the cond-cov branch from 80a1cda to c88943f Compare May 5, 2023 14:59

Add conditional coverage flags for accurate platform coverage

f95607c

rmartin16 force-pushed the cond-cov branch from c88943f to f95607c Compare May 5, 2023 18:46

rmartin16 mentioned this pull request May 5, 2023

Conditional Coverage Reporting with Platform Spoofing #1262

Merged

4 tasks

rmartin16 closed this May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conditional coverage flags for accurate platform coverage #1260

Add conditional coverage flags for accurate platform coverage #1260

rmartin16 commented May 3, 2023

rmartin16 commented May 5, 2023 •

edited

Loading

freakboy3742 commented May 5, 2023 •

edited

Loading

freakboy3742 May 5, 2023

rmartin16 May 5, 2023

rmartin16 May 5, 2023

rmartin16 May 5, 2023

rmartin16 May 5, 2023

rmartin16 May 5, 2023

rmartin16 commented May 6, 2023

Add conditional coverage flags for accurate platform coverage #1260

Add conditional coverage flags for accurate platform coverage #1260

Conversation

rmartin16 commented May 3, 2023

PR Checklist:

rmartin16 commented May 5, 2023 • edited Loading

freakboy3742 commented May 5, 2023 • edited Loading

freakboy3742 May 5, 2023

Choose a reason for hiding this comment

rmartin16 May 5, 2023

Choose a reason for hiding this comment

rmartin16 May 5, 2023

Choose a reason for hiding this comment

rmartin16 May 5, 2023

Choose a reason for hiding this comment

rmartin16 May 5, 2023

Choose a reason for hiding this comment

rmartin16 May 5, 2023

Choose a reason for hiding this comment

rmartin16 commented May 6, 2023

rmartin16 commented May 5, 2023 •

edited

Loading

freakboy3742 commented May 5, 2023 •

edited

Loading